lc trie: return a vector of tags by ccaraman · Pull Request #2612 · envoyproxy/envoy

ccaraman · 2018-02-14T21:07:35Z

Signed-off-by: Constance Caramanolis ccaramanolis@lyft.com
Description:
The LC-Trie would only return a single tag because nested prefixes was not supported yet. To unblock nested prefixes, the getTag methods have been changed to getTags and return a vector of strings.

Risk Level: Low - change interface for class that isn't consumed yet.

Testing: Existing unit tests have been updated to expect vectors.

Docs Changes: N/A

Release Notes: N/A

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

jmarantz

drive-by comment really to help me learn what the style is supposed to be :)

jmarantz · 2018-02-15T00:36:42Z

source/common/network/lc_trie.h

-   * @return tag from the CIDR range that contains 'ip_address'. An empty string is returned
-   *             if no prefix contains 'ip_address' or there is no data for the IP version of the
-   *             ip_address.
+   * @return a vector of tags from the CIDR ranges and IP addresses that contains 'ip_address'. An


I thought the pattern here was to give the type immediately after the @param or @return keyword, e.g.

* @return std::vector<std::string> a vector of tags ...

at least I've seen this elsewhere in the codebase. I'm not sure what script or program processes this style though.

This is a good question. I don't know what peoples preferences are these days. I'll find out.

In our local chatter, no one understands why we are repeating the types in the comments; and I learned they are not needed for any scripts that generate API doc currently.

IMO your form is better and we should convert everything to it.

This has come up a couple times. Matt commented that he started the policy out of habit, because some script (super old doxygen from 1973??) output better docs if it was included.

I think we should remove those types from all the docs. They're not providing any value, and they can get out-of-date from the actual types on the function.

If not, we should make a linter that enforces them.

Agreed, i'll keep it as is.

Possibly worth a call-out on envoy-dev or maintainers - I'm so happy to kill off that rule, and hadn't realized it was "outdated" so was enforcing.

dnoe · 2018-02-15T15:04:23Z

Assigned @junr03 for first maintainer pass since he reviewed the previous LC-Trie PR.

@brian-pane Can you also do a review of this here, since I see you are working with this as well? Thanks

brian-pane · 2018-02-15T16:23:39Z

This change looks good to me. The one change I can think of would be to replace the std::vector return type with something like Folly's small_vector or Abseil's inlined_vector that doesn't need a heap allocation for the common case where the number of returned tags is small. But those have a drawback: the maximum expected vector size is encoded as a template parameter, and that's a detail that probably shouldn't be exposed in the trie's API.

brian-pane · 2018-02-15T16:26:30Z

It looks like the TSAN test failure is due to the (unrelated) bug that was fixed by #2613, so merging master should fix it.

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

ccaraman · 2018-02-15T19:12:05Z

@brian-pane 1. I'm going to leave it as is with std::vector<> I don't want to modify the template parameter. When benchmark testing is added for LC-Trie, we can see if there are any issues with vector. 2. merged master

alyssawilk

Thanks for splitting the refactor out from the upcoming improvements - It's nice and clean and soo much easier to review!

LGTM overall, just a few nits

alyssawilk · 2018-02-15T20:11:35Z

source/common/network/lc_trie.h

-   * @return tag from the CIDR range that contains 'ip_address'. An empty string is returned
-   *             if no prefix contains 'ip_address' or there is no data for the IP version of the
-   *             ip_address.
+   * @return a vector of tags from the CIDR ranges and IP addresses that contains 'ip_address'. An


Possibly worth a call-out on envoy-dev or maintainers - I'm so happy to kill off that rule, and hadn't realized it was "outdated" so was enforcing.

alyssawilk · 2018-02-15T20:13:01Z

source/common/network/lc_trie.h

  if (ip_prefixes_[address].length_ == 0) {
-    return ip_prefixes_[address].tag_;
+    return_vector.push_back(ip_prefixes_[address].tag_);
+    // TODO(ccaraman): When nested prefixes are supported, should the /0 case be handled better.


super nitty nit - either swap . for ? or make it a statement please

:) - I'll change it to a '?'.

alyssawilk · 2018-02-15T20:17:13Z

test/common/network/lc_trie_test.cc

+
+  void expectIPAndNoTag(const std::vector<std::string>& test_output) {
+    for (const auto& ip : test_output) {
+      EXPECT_EQ(0, trie_->getTags(Utility::parseInternetAddress(ip)).size());


My only thought on the test refactor is that if multi-vector responses are coming, you may want to refactor expectIPAndTag to actually take a vector, rather than splitting this into "one string" "empty vector" and "multi-string response" test cases. Your call if it's easier this way.

I was able to implement #2614 on top of this change by merging multiple results into a single vector. I think that will suffice for the IP tagging filter.

I got rid of expectIPandNoTag() and kept ExpectIPandTags(). It checks the content of the tag vector against the expected vector, empty vectors are supported too!

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

ccaraman added 2 commits February 14, 2018 13:03

Fix getTag to be getTags and return a vector of strings

a1ab29d

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

Add another todo

681c06a

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

jmarantz reviewed Feb 15, 2018

View reviewed changes

brian-pane mentioned this pull request Feb 15, 2018

Support nested prefixes in the LcTrie class #2614

Merged

dnoe assigned junr03 Feb 15, 2018

Merge branch 'master' into ret_vector

041791e

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

alyssawilk reviewed Feb 15, 2018

View reviewed changes

Address a few comments.

a2e1a6e

Signed-off-by: Constance Caramanolis <ccaramanolis@lyft.com>

dnoe assigned alyssawilk Feb 16, 2018

alyssawilk approved these changes Feb 20, 2018

View reviewed changes

alyssawilk merged commit 947fa6a into master Feb 20, 2018

ccaraman deleted the ret_vector branch February 23, 2018 18:31

brian-pane mentioned this pull request Jan 4, 2019

rewrite buffer implementation to eliminate evbuffer dependency #5441

Merged

Conversation

ccaraman commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnoe commented Feb 15, 2018

Uh oh!

brian-pane commented Feb 15, 2018

Uh oh!

brian-pane commented Feb 15, 2018

Uh oh!

ccaraman commented Feb 15, 2018

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ccaraman commented Feb 14, 2018 •

edited

Loading